feat: run integration tests on more platforms#760
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds RHEL and RHEL-ARM64 platforms to the integration test matrix, configures Podman/Docker runtimes via Ansible group variables, restructures test execution to clone and run tests within the target VM using pytest and gRPC stubs, and updates test logic for cross-version compatibility. ChangesMulti-Platform Integration Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
0900afe to
0e9261a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #760 +/- ##
=======================================
Coverage 27.96% 27.96%
=======================================
Files 21 21
Lines 2596 2596
Branches 2596 2596
=======================================
Hits 726 726
Misses 1867 1867
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3ec1fe4 to
f6346aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/integration-tests.yml (1)
77-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize the Quay credentials instead of embedding them raw in YAML.
Lines 83-85 write the secret values straight into
vars.yml. A valid username/password containing:,#,{},*, or a newline can make the file unparsable or change the parsed scalar, which breaks the later-e@vars.yml`` calls before the playbooks run. Build this file with a serializer instead (for example, emit JSON with Python and pass that file to-e @...) or robustly escape the values before writing YAML.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/integration-tests.yml around lines 77 - 94, Secrets are being written raw into vars.yml which can break YAML parsing for special characters; instead serialize credentials safely (e.g., emit JSON) and consume that file. Replace the here-doc that writes vars.yml with a step that generates a serialized file (JSON or safely escaped YAML) using a small Python step that reads JOB_ID, FACT_IMAGE_NAME, FACT_VERSION and the GitHub secrets (secrets.QUAY_RHACS_ENG_RO_USERNAME / secrets.QUAY_RHACS_ENG_RO_PASSWORD) and writes a vars.json, then update downstream usage from -e `@vars.yml` to -e `@vars.json` (or keep `@vars.yml` if you serialize valid YAML); ensure the producer uses json.dump (or a proper YAML serializer) so characters like :, #, { } and newlines are escaped correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/integration-tests.yml:
- Around line 123-127: The for-loop "for file in logs/*.tar.gz; do" will run
once with the literal pattern if no files exist; fix by either enabling bash
nullglob before the loop (shopt -s nullglob; ...; shopt -u nullglob) so the
pattern expands to an empty list, or guard the loop with a compgen check
(compgen -G 'logs/*.tar.gz' >/dev/null || true && for file in logs/*.tar.gz; do
...) so the loop only runs when matches exist; apply the change around the
existing cd "/tmp/fact/tests" and the tar xzf/rm sequence to avoid tar trying to
extract "logs/*.tar.gz".
In `@ansible/group_vars/platform_rhcos.yml`:
- Line 3: The platform RHCOS/RHEL group vars override the socket but not the
runtime command, so tasks still take the Docker path; update
ansible/group_vars/platform_rhcos.yml,
ansible/group_vars/platform_rhcos_arm64.yml,
ansible/group_vars/platform_rhel.yml, and
ansible/group_vars/platform_rhel_arm64.yml to also set runtime_command: podman
(or alternatively add that override into a shared Podman-specific group file
used by those platforms) so run-tests.yml and the Podman auth-copy task detect
and use podman instead of docker.
In `@ansible/run-tests.yml`:
- Around line 78-93: The fetch/archive tasks ("Retrieve results", "Compress log
files", "Fetch log files") assume artifacts always exist; add an
ansible.builtin.stat for /tmp/fact/tests/results.xml and for
/tmp/fact/tests/logs before these tasks, then make each task conditional (when:
stat_result.stat.exists) so they only run if present; for fetch tasks set
fail_on_missing: false (and keep validate_checksum: false) so missing artifacts
don’t mask the real failure from the earlier shell block.
In `@tests/test_path_rmdir.py`:
- Around line 158-161: The test currently uses pytest.mark.skipif to skip the
test on Python < 3.10, which contradicts the PR description claiming the test
reverses event order for those versions; update the implementation to match the
description by removing the skip and adding conditional logic in
tests/test_path_rmdir.py (around the pytest.mark.skipif and the test_path_rmdir
test) to reverse the expected shutil.rmtree event order when sys.version_info <
(3, 10), or alternatively change the PR description and test comment to
explicitly state that the test is intentionally skipped on Python < 3.10; choose
one approach and make the code and description consistent.
---
Outside diff comments:
In @.github/workflows/integration-tests.yml:
- Around line 77-94: Secrets are being written raw into vars.yml which can break
YAML parsing for special characters; instead serialize credentials safely (e.g.,
emit JSON) and consume that file. Replace the here-doc that writes vars.yml with
a step that generates a serialized file (JSON or safely escaped YAML) using a
small Python step that reads JOB_ID, FACT_IMAGE_NAME, FACT_VERSION and the
GitHub secrets (secrets.QUAY_RHACS_ENG_RO_USERNAME /
secrets.QUAY_RHACS_ENG_RO_PASSWORD) and writes a vars.json, then update
downstream usage from -e `@vars.yml` to -e `@vars.json` (or keep `@vars.yml` if you
serialize valid YAML); ensure the producer uses json.dump (or a proper YAML
serializer) so characters like :, #, { } and newlines are escaped correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b5840b5-b53d-44ae-9379-c321d345e575
📒 Files selected for processing (13)
.github/workflows/integration-tests.ymlCHANGELOG.mdansible/ansible.cfgansible/group_vars/all.ymlansible/group_vars/platform_rhcos.ymlansible/group_vars/platform_rhcos_arm64.ymlansible/group_vars/platform_rhel.ymlansible/group_vars/platform_rhel_arm64.ymlansible/run-tests.ymltests/event.pytests/test_config_hotreload.pytests/test_path_rename.pytests/test_path_rmdir.py
Stringy
left a comment
There was a problem hiding this comment.
Minor question/suggestion but non-blocking; otherwise LGTM
|
stackrox/collector#3152 was merged recently, adding some newer RHCOS kernels, I'll do some more testing before I merge the PR. |
Add support for running integration tests on Python 3.9 through 3.14+ with minimal changes using modern Python features. Changes: - event.py: Add __future__ annotations import to enable modern type hint syntax (int | None) on Python 3.9, with fallback for @OverRide decorator (Python 3.12+) - test_config_hotreload.py: Handle both builtin TimeoutError (Python 3.11+) and concurrent.futures.TimeoutError (Python 3.9-3.10) - test_path_rmdir.py: Reverse event order for Python < 3.10 due to different shutil.rmtree deletion ordering This avoids using deprecated Union/Optional types while maintaining backward compatibility. Tested: 102 tests pass on Python 3.9.25 Assisted-by: Claude Code (claude-sonnet-4-5@20250929)
This fixes a connectivity issue with RHEL 10 VMs that only happens on CI.
a374e7e to
6a48c1f
Compare
|
I added in RHCOS 10 on Arm and the integration test works. I'll merge as is since there are no major changes since the last approval. |
After adding some more test platforms through #760, some tests have started failing with the claim that fact failed to start. Upon reviewing the logs it turns out fact sometimes take slightly longer to start, so increasing the number of times we retry the health_check endpoint should be enough to mitigate this flake.
Description
Add support for running integration tests on Python 3.9 through 3.14+ with minimal changes using modern Python features.
Changes:
test_rmdir_recursivefor Python < 3.10 due to different and inconsistent shutil.rmtree deletion orderingThis avoids using deprecated Union/Optional types while maintaining backward compatibility.
Assisted-by: Claude Code (claude-sonnet-4-5@20250929)
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Tested: 102 tests pass on Python 3.9.25
Summary by CodeRabbit
New Features
CI / Testing Infrastructure
Configuration
Tests